Conversation
mergify
Bot
added
scope: inference
|
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI
🔴 PR merge requirementsWaiting for
This rule is failing.
|
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in the MLX request validation path (task handling) and the PR also introduces a new serving/runtime surface area that still needs real-hardware validation before it’s safe to merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends FastVideo’s OpenAI-compatible serving stack to support a non-CUDA runtime (MLX) by allowing the server to plug in an alternate generator implementation and runtime-specific request validation, then adds a native MLX “FastMetal Wan” server/pipeline for 1.3B, 14B, and 5B.
Changes:
- Add a pluggable
generator_factory+ per-runtimevideo_request_validatorto the shared OpenAI server app/engine so non-CUDA backends can be served. - Introduce MLX Wan pipelines (Wan2.1 for 1.3B/14B and Wan2.2-TI2V for 5B) plus a dedicated MLX Wan server entrypoint.
- Add tests and example YAML configs covering config parsing, validation, and generator dispatch for all three model sizes.
File summaries
| File | Description |
|---|---|
| fastvideo/entrypoints/openai/api_server.py | Adds runtime/generator factory hooks and conditionally disables image routes for MLX runtime. |
| fastvideo/entrypoints/openai/serving_engine.py | Generalizes generator shape via ServingGenerator and adds runtime-specific request validation hook. |
| fastvideo/entrypoints/openai/state.py | Updates global generator typing to the new serving generator protocol. |
| fastvideo/entrypoints/openai/video_api.py | Runs runtime-specific request validation before CUDA-oriented model/LoRA validation. |
| fastvideo/entrypoints/openai/mlx_wan_server.py | New MLX Wan server entrypoint, config parsing, request allowlist validation, and MLX generator implementation. |
| fastvideo/mlx_runtime/wan_pipeline.py | New MLX Wan2.1 and Wan2.2-TI2V pipelines and shared prompt/rope helpers for repeated server calls. |
| fastvideo/tests/entrypoints/test_mlx_wan_server.py | Tests MLX Wan server config parsing, allowlist validation, and generator dispatch/routing. |
| fastvideo/tests/mlx/test_mlx_wan_pipeline.py | Tests MLX Wan pipeline constructors’ filesystem + checkpoint-shape validation. |
| examples/serving/mlx_wan21_1_3b.yaml | Example serving config for FastMetal Wan2.1 1.3B. |
| examples/serving/mlx_wan21_14b.yaml | Example serving config for FastMetal Wan2.1 14B. |
| examples/serving/mlx_wan22_5b.yaml | Example serving config for FastMetal Wan2.2-TI2V 5B. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if request.task not in (None, "t2v"): | ||
| raise ValueError("Wan MLX serving supports task=t2v only.") |
| try: | ||
| manifest = json.loads(manifest_path.read_text()) | ||
| except (json.JSONDecodeError, OSError): | ||
| return None | ||
| config = manifest.get("config", manifest) | ||
| channels = config.get("in_channels") | ||
| return int(channels) if channels is not None else None |
| def get_generator() -> ServingGenerator: | ||
| """Return the global VideoGenerator instance (set during startup).""" | ||
| assert _generator is not None, "Server not initialized — generator is None" | ||
| return _generator |
61bf7c4 to
56ef624
Compare
|
Rebased onto main to pick up #1798’s shared MLX serving infrastructure; no content changes (range-diff clean). |
| "height", | ||
| "fps", | ||
| "num_frames", | ||
| "seconds", |
There was a problem hiding this comment.
[P1] Align or reject seconds before admitting the job. The shared adapter turns an explicit seconds into num_frames = seconds * fps; with both shipped defaults (16 and 24 fps), that is always 0 mod 4, while plan_refine_resolutions requires Wan frames to be 1 mod 4. A normal OpenAI-style request therefore gets queued and fails only inside generation. Please validate the merged request shape synchronously and either map duration to the nearest valid frame grid (for example seconds * fps + 1) or do not advertise seconds for this runtime.
There was a problem hiding this comment.
Fixed in 11f0c76. validate_wan_video_request now resolves an explicit seconds to a Wan-legal num_frames before the job is admitted, rounding up to the next value that is 1 mod the VAE temporal stride. It mirrors the adapter's explicit-field precedence, including the nested video_params spelling, and leaves an explicit num_frames untouched. create_mlx_wan_app binds the served fps into the validator so alignment uses the config's fps rather than the adapter's generic 24 fallback.
Verified on an M1 Pro: {"seconds": 1, "size": "256x256"} produced 17 frames (ffprobe nb_read_frames=17) where the naive 1×16 = 16 would have been rejected. A {"seconds": 3} request at 480×832 resolved to 49 frames and cleared plan_refine_resolutions plus all three denoise steps; it then hit a Metal OOM in TAEHV decode, which is a memory limit on this 16 GB machine rather than the admission path. Full logs in the comment below.
| tokenizer = AutoTokenizer.from_pretrained(model_root / "tokenizer", local_files_only=True) | ||
| text_encoder = UMT5EncoderModel.from_pretrained( | ||
| model_root / "text_encoder", | ||
| torch_dtype=torch.bfloat16, |
There was a problem hiding this comment.
[P2] Keep the 5B prompt-encoder path on the recipe-validated dtype/device, or prove the new path on hardware. mlx_wan22_generate.py deliberately encodes 5B prompts in FP16 on CPU, but this shared helper forces BF16 on MPS and only casts the already-rounded embeddings back to FP16 afterward. That is not the same math (BF16 loses three mantissa bits), and this PR has no real-Mac generation/parity run to show the 5B output remains valid. Please parameterize dtype/device by model family and retain the 5B FP16 path, with an actual FastMetal-5B smoke/parity result.
There was a problem hiding this comment.
Fixed in 11f0c76. _encode_wan_prompt now takes device_arg and dtype_arg, defaulting to Wan2.1's recipe (bf16 / auto, matching mlx_wan_prompt_to_video.py). MLXWan22Pipeline passes cpu / fp16, so 5B is back on the path mlx_wan22_generate.py validated. The bf16 widening before the NumPy hand-off is now conditional, since fp16 maps directly.
Parity result on an M1 Pro — server path vs. the reference script, same prompt, both on the 5B recipe:
server: torch.float16 (1, 512, 4096)
script: torch.float16 (1, 512, 4096)
bit-identical: True
max abs diff: 0.0
SolitaryThinker
left a comment
SolitaryThinker
left a comment
There was a problem hiding this comment.
Requesting changes for the two inline blockers and the still-open task-validation mismatch:
- An explicit seconds request is admitted but the shared adapter produces a frame count that violates the Wan 1-mod-4 temporal grid under both shipped FPS defaults.
- The 5B server changes the maintained FP16/CPU prompt-encoder path to BF16/MPS without real-hardware parity evidence.
- validate_wan_video_request accepts task=t2v, but the shared request adapter rejects every non-None task for non-MiniMax models, as the existing inline review notes.
Please also address the malformed-manifest validation comment, run at least one real Apple-Silicon generation for each distinct pipeline (Wan2.1 and Wan2.2), and prefix the PR title with [feat] so merge protection can pass. Changed-file pre-commit is green on the rebased head; the focused pytest collection is not runnable on this Linux host because package import initializes Triton without an active GPU driver.
Ishxn20
changed the title
SolitaryThinker
left a comment
SolitaryThinker
left a comment
There was a problem hiding this comment.
Deep review — correctness / efficiency / simplification
Reviewed head 11f0c76b8. CI is green; the standing change request is unchanged. Findings ordered by severity, with file:line evidence.
Blocking
1. Wan2.2 (5B) has no end-to-end hardware evidence.
fastvideo/mlx_runtime/wan_pipeline.py:352 (MLXWan22Pipeline.generate), examples/serving/mlx_wan22_5b.yaml. The PR body still marks "5B (Wan2.2) generation not yet run", and the only 5B evidence is a prompt-encoder parity check. The 48-channel DiT load, sample_wan22_dmd (flow_shift 5.0, renoise seed 0), and z_dim=48 TAEHV decode have never executed. This is the outstanding item from the previous review; it needs Apple Silicon hardware with more memory than the author's 16 GB M1 Pro.
Major
2. Grid-invalid explicit geometry is admitted, then fails inside generation after a full UMT5 encode.
fastvideo/entrypoints/openai/mlx_wan_server.py:115 (validate_wan_video_request) does not check num_frames % 4 == 1 or width/height divisibility; wan_pipeline.py:241/:380 encode the prompt before plan_refine_resolutions at :246/:387. Verified locally: num_frames=80 and size=833x481 both pass validation, then fail inside generation (async: generation_failed; /v1/videos/sync: HTTP 500) after paying the ~45 s UMT5 load/encode. This is the synchronous-rejection contract the validator docstring and the prior P1 asked for.
3. fastvideo serve --config cannot load the shipped MLX YAMLs.
Verified: build_serve_config on examples/serving/mlx_wan21_1_3b.yaml raises ConfigValidationError: runtime: unknown field (fastvideo/api/schema.py:269 ServeConfig has no runtime; GeneratorConfig has no model_root/mlx_checkpoint). The real entrypoint is python -m fastvideo.entrypoints.openai.mlx_wan_server --config ... (consistent with H3's mlx_server), but the title, the body ("1.3B served via fastvideo serve"), and examples/serving/README.md:40-45 never say so.
4. Prompt cache is bypassed; every request reloads UMT5 and re-encodes.
wan_pipeline.py:87 (_encode_wan_prompt) never calls load_prompt_cache/save_prompt_cache, and mlx_wan_server.py:47-52 has no cache field. The H3 server wires prompt_cache_dir (mlx_server.py:32, minimax_h3_pipeline.py:401), and both CLI recipes use the cache by default. The repo's own comment (examples/inference/basic/mlx_wan22_generate.py:109) measures a full UMT5 encode at ~45 s on an M4 Max — paid on every request, including identical playground re-runs.
5. Playground claim is false for Wan.
mlx_wan_server.py:2 says "through the shared video-job API and playground", but fastvideo/entrypoints/openai/playground.py:25-33 (require_h3) 404s every non-H3 family. /playground/ is dead for FastMetal.
6. Unregistered FastMetal paths cause HF Hub lookups per request and in unit tests.
request_adapter.py:339 / api/sampling_param.py:212 → registry.py:216 → hf_hub_download. Verified: build_generation_request issued HEAD+GET for FastVideo/FastMetal-1.3B-QAD/model_index.json during a local request. FastMetal has no registry.py entry (unlike FastH3), so ~3 lookups per request plus 2 at startup; offline machines stall on retry timeouts before falling back to SamplingParam().
7. _align_seconds_to_frame_grid diverges from the adapter on fps: null + video_params.fps.
mlx_wan_server.py:86-112 vs request_adapter.py:300-317. Verified: with seconds=1, fps=None, video_params={"fps": 16}, the validator resolves num_frames=17 from fps 16, while the adapter then uses its 24 fallback → 17 frames at 24 fps. Admission and generation disagree on the same request.
8. No test exercises either generate() orchestration path.
fastvideo/tests/mlx/test_mlx_wan_pipeline.py covers only __init__ filesystem/architecture guards; test_mlx_wan_server.py:239-336 stubs the pipeline class. The per-family contracts this PR introduces (bf16/MPS vs fp16/CPU, flow_shift 8.0 vs 5.0, DMD ladder, renoise seed 0, z_dim 16 vs 48) are unverified.
Minor
wan_pipeline.py:292-293:latents.astype(mx.float32)computed twice per step;dmd_stepdiscards thelatentsarg (sampling.py:131) — one wasted full-tensor cast/step.mlx_wan_server.py:27,31duplicate_DEFAULT_DMD_STEPS/temporal-stride constants with nothing linking them.wan_pipeline.py:61callsmx.get_peak_memory()unguarded; H3 guards withgetattr(minimax_h3_pipeline.py:206).wan_pipeline.py:54defines a thirdGenerationResult, shadowing the one exported fromfastvideo/mlx_runtime/__init__.py:71.mlx_wan_server.pyshares 140 identical lines withmlx_server.py;_encode_wan_prompt/_make_wan_rotary_embeddingscopy the CLI helpers, contradicting the module docstring's "cannot silently drift" claim.- Stale comments:
api_server.py:169-170("video-with-audio" is H3-only),wan_pipeline.py:69device docstring,mlx_wan_server.py:57-60self-aware TODO. examples/serving/README.md:40-45documents onlymlx_server; the three new configs are undiscoverable._make_wan_rotary_embeddings(wan_pipeline.py:152) indexesconfig["patch_size"]directly (bareKeyError; CLI helper does the same).- DiT reload +
mx.compilere-trace per request andtimings/peak_memory_gibcomputed then dropped — both deliberate and matching H3's phase-memory policy.
Verified correct
- 5B encoder is back on the recipe-validated fp16/CPU path; 1.3B/14B stay bf16/auto (
wan_pipeline.py:379-382). secondsalignment mirrors the adapter's explicit-field precedence for the common spellings;task=t2vis normalized before the adapter rejects it._packed_dit_channels(wan_pipeline.py:170-194) handles malformed manifests (the earlier Copilot comment).- Model→pipeline dispatch, non-Apple/ffmpeg rejection, worker-thread lifecycle, and
__init__architecture guards are covered. - Shared serving files changed only in comments/docstrings; the
runtime != "mlx"image-router exclusion was already onmain. - CI coverage:
test_mlx_wan_pipeline.pyis in both macOS jobs;test_mlx_wan_server.pyruns in the Buildkite unit lane.
Could not verify
Real Apple Silicon generation (no Mac/MLX here), the 5B DiT/sampler/decode path, and whether mx.get_peak_memory is guaranteed by the pinned MLX floor.
cc @aryan5v for the 5B hardware validation.
|
Rebased locally onto current Could not complete 5B (Wan2.2) generation here. Not merge-ready. Two things I re-checked and would not ship without:
Seconds → frame-grid alignment does survive into |
Summary
Testing
358 automated tests pass — config loading, bad-request rejection, and routing to the right model size.
Confirmed the existing Nvidia server path is untouched.
Real generation on Apple Silicon (M1 Pro, 16 GB). 1.3B served via
fastvideo serve, generated end to end on Metal:fox.mp4
5B prompt-encoder parity — bit-identical. Compared the server's
_encode_wan_promptagainst the reference script'sencode_promptfor thesame prompt on the 5B recipe (fp16 / CPU):
5B (Wan2.2) generation not yet run — needs more unified memory than this
machine has; a 49-frame 1.3B decode already exhausted 16 GB. Pending a
larger Mac. (@aryan5v)